-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
address issue #282 #283
address issue #282 #283
Conversation
added pixi.toml with depends as well as build tasks, made CONTRIBUTING.md mostly added stuff regarding dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I've got a few notes:
- When writing markdown can you make sure that each sentence is on a newline. This makes diffs a lot more readable/managable.
- More inline in the code :)
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments. To split sentences over lines in markdown and to also use ~=
for the other pixi dependencies. Otherwise looks really good. 👍
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
Thanks for putting those in, hadn't realized the first set of edits you suggested weren't all the instances needing change. |
Also, is there a preference to squash commits so that the PR has fewer commits where reasonable or is it typically kept as part of the history? |
I usually squash and merge when I merge the PR. |
Thanks @YeungOnion! Fantastic first contribution! |
addresses #282
ran a pixi init and added depends listed in environments.yml.
Needing feedback on
pixi add ...